New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add app.getLocaleCountryCode() method for region detection #15035
Conversation
💖 Thanks for opening this pull request! 💖 We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix. Examples of commit messages with semantic prefixes:
Things that will help get your PR across the finish line:
We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can. |
@zarubond can you please rebase your branch onto the latest |
27c16d8
to
a08e050
Compare
atom/browser/api/atom_api_app.cc
Outdated
region = locale.substr(rpos + 1, 2); | ||
} | ||
#endif | ||
if (region.size() == 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this is probably cleaner as a ternary:
return region.size() == 2 ? region : std::string();
atom/browser/api/atom_api_app.cc
Outdated
static_cast<CFTypeRef>(CFLocaleGetValue(locale, kCFLocaleCountryCode))); | ||
const CFIndex kCStringSize = 128; | ||
char temporaryCString[kCStringSize]; | ||
bzero(temporaryCString, kCStringSize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or just char temporaryCString[kCStringSize] = {}
docs/api/app.md
Outdated
@@ -580,6 +580,10 @@ To set the locale, you'll want to use a command line switch at app startup, whic | |||
|
|||
**Note:** On Windows you have to call it after the `ready` events gets emitted. | |||
|
|||
### `app.getRegion()` _macOS_ _Linux_ _Windows_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For functions that work across platforms, no need to specify all of _macOS_ _Linux_ _Windows_
@zarubond could you please add spec(s) for this, as well as release notes? |
atom/browser/api/atom_api_app.cc
Outdated
@@ -1299,7 +1338,8 @@ void App::BuildPrototype(v8::Isolate* isolate, | |||
.SetMethod("setPath", &App::SetPath) | |||
.SetMethod("getPath", &App::GetPath) | |||
.SetMethod("setDesktopName", &App::SetDesktopName) | |||
.SetMethod("getLocale", &App::GetLocale) | |||
.SetMethod("getRegion", &App::GetRegion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getRegion
should be removed now i believe since you changed the name
docs/api/app.md
Outdated
@@ -580,6 +580,11 @@ To set the locale, you'll want to use a command line switch at app startup, whic | |||
|
|||
**Note:** On Windows you have to call it after the `ready` events gets emitted. | |||
|
|||
### `app.GetLocaleCountryCode()` | |||
Returns `String` - User operating system´s region in ISO3166 [here](https://www.iso.org/iso-3166-country-codes.html). The value is taken from native OS APIs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
region
=> locale country code
docs/api/app.md
Outdated
### `app.GetLocaleCountryCode()` | ||
Returns `String` - User operating system´s region in ISO3166 [here](https://www.iso.org/iso-3166-country-codes.html). The value is taken from native OS APIs. | ||
|
||
**Note:** When unable to detect region, it returns empty string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above ☝️
docs/api/app.md
Outdated
@@ -580,6 +580,11 @@ To set the locale, you'll want to use a command line switch at app startup, whic | |||
|
|||
**Note:** On Windows you have to call it after the `ready` events gets emitted. | |||
|
|||
### `app.GetLocaleCountryCode()` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app.GetLocaleCountryCode()
=> app.getLocaleCountryCode()
@zarubond spec is failing on |
@codebytere Would it be possible to get output of "locale" command on Linux testing machine? |
@zarubond This is the output of
Doesn't look too good 😆 |
Thanks @MarshallOfSound , so the output of the code was correct. |
@zarubond |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^
@zarubond It should be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍
Release Notes Persisted
|
Congrats on merging your first pull request! 🎉🎉🎉 |
…ctron#15035) * Add method to get system´s user region * Fix linter * Remove auto types * Improved detection for POSIX * Change name, add specs, minor fixes * Remove left overs * Fix locale test * Fix Linux test * Coding style fixes * Fix docs * Add test excaption for Linux * fix spelling * Polishing
Description of Change
On desktop operating systems the users can set different language and locale region. This allows the system to have en-CZ locale even through there is no czech region associated with english (the example results in English language with Czech data/number format/currency). However Chrome only presents region in locale based on system language.
This patch adds the API returning the OS region which can be then used for globalization. With the knowledge of region the applications would be able to show proper localization as any other native app.
Checklist
Release Notes
Notes: feat: provide user system's region with
app.getLocaleCountryCode()